Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

For a summary:

This extends std::io::Read by allowing users to implement Read by only providing a Read::read_buf impl, and not a Read::read impl. It does this using the unstable attribute rustc_must_implement_one_of¹.

#[rustc_must_implement_one_of(read, read_buf)]
pub trait std::io::Read {
    // (new)
    fn read(&mut self, buf: &mut [u8]) -> Result<usize> {
        let mut buf = BorrowedBuf::from(buf);
        self.read_buf(buf.unfilled()).map(|()| buf.len())
    }
    // (existing)
    fn read_buf(&mut self, buf: BorrowedCursor<'_>) -> Result<()> {
        default_read_buf(|b| self.read(b), buf)
    }
}

This does not make rustc_must_implement_must_of user-visible, since read_buf is unstable (altho IMO it could easily be forgotten when stabilizing).

Whether or not we want to start using the #[rustc_must_implement_one_of(...)] attribute on the stdlib is a question that's split between lang (if they think the language feature has a path to stabilization) and libs-api (if they are okay with using it in that case). I'm on neither of these teams, and don't feel that strongly (but weakly, I think it probably needs a t-libs-api meeting to discuss after asking t-lang if it's okay start experimenting with)

r? @joshtriplett (who is on both relevant teams)

I'm nominating this for T-lang to consider that question: are we OK with people using rustc_must_implement_one_of in stable standard-library APIs?

Separately, I'm nominating this for libs-api. Are we OK with (on nightly) allowing people to implement read_buf and not read?

I am in favor, and in the absence of objections I'd r+ this once we have T-lang confirmation.

Discussed this asynchronously with some T-lang folks, and one item that came up: we'd really like to see rustc_must_implement_one_of at least documented in the Rust reference. And once it's documented, we may well be up for just stabilizing it as must_implement_one_of.

I've created a tracking issue: #107460

Removing I-libs-api-nominated for now, until the attribute has documentation in the reference.

We discussed this once more in @rust-lang/lang, and confirmed that we have no objection to adding this on nightly.

@bors r+

We'd consider it a blocker for stabilization of read_buf that the attribute be either stabilized or at least documented in the reference or similar.

📌 Commit d7dac91 has been approved by joshtriplett

It is now in the queue for this repository.

I am concerned about this negatively affecting the experience of non-nightly users. For example with this code:

struct MyRead;

impl std::io::Read for MyRead {}

current stable gives you this error:

error[E0046]: not all trait items implemented, missing: `read`
 --> src/main.rs:3:1
  |
3 | impl std::io::Read for MyRead {}
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `read` in implementation
  |
  = help: implement the missing item: `fn read(&mut self, _: &mut [u8]) -> Result<usize, std::io::Error> { todo!() }`

whereas rustc built from this PR gives this error:

error[E0046]: not all trait items implemented, missing one of: `read`, `read_buf`
   --> src/main.rs:3:1
    |
3   | impl std::io::Read for MyRead {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing one of `read`, `read_buf` in implementation
    |
note: required because of this annotation
   --> /git/rust/library/std/src/io/mod.rs:552:1
    |
552 | #[rustc_must_implement_one_of(read, read_buf)]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Some concerns:

• I only know how to build rustc in dev mode. When it's built in stable mode, is it still going to tell the user to consider implementing the unstable read_buf method, which they cannot do on a stable toolchain? That would be undesirable and it would be good to have a UI test covering this case.

• Notice that even aside from "missing read" vs "missing one of read or read_buf", the new error is less helpful because it does not give you the signature you need to put, so there isn't a signature you can copy straight from the error into your code. Would it make sense to give 2 "help:", one with each signature?

• How does this impact rust-analyzer? When you write a trait impl and tell it to autofill the required methods, is it going to put both? neither? just the alphanumerically first one or the one on the lower line number in libstd? My sense is a lot of people rely on this autofill when writing trait impls in an IDE.

@bors r-

When it's built in stable mode, is it still going to tell the user to consider implementing the unstable read_buf method, which they cannot do on a stable toolchain? That would be undesirable and it would be good to have a UI test covering this case.

Valid; I agree that we should avoid suggesting implementing a method that you can't implement.

@WaffleLapkin, would that be feasible, and can you add a test verifying that behavior as well?

Notice that even aside from "missing read" vs "missing one of read or read_buf", the new error is less helpful because it does not give you the signature you need to put, so there isn't a signature you can copy straight from the error into your code. Would it make sense to give 2 "help:", one with each signature?

Fair. That seems like a reasonable solution for the common case.

How does this impact rust-analyzer? When you write a trait impl and tell it to autofill the required methods, is it going to put both? neither? just the alphanumerically first one or the one on the lower line number in libstd? My sense is a lot of people rely on this autofill when writing trait impls in an IDE.

@dtolnay thanks for rising these concerns! My guess is that r-a will not autofill any methods, as they all have default bodies. I'll look into how we can support this feature in r-a.

WaffleLapkin, would that be feasible, and can you add a test verifying that behavior as well?

@joshtriplett This should be feasible. I'll make a PR that removes mentions of unstable methods from this diagnostic and shows the signatures soon™.

For now, marking this PR as blocked.

@WaffleLapkin I needed to update it in order to test it with #160106 anyway.

I don't think we necessarily need to block on improving the rust-analyzer suggestions here. I think with the improved suggestions (now merged), and if I add support for not suggesting unstable methods (in progress), that would largely solve the problem. We can ping the rust-analyzer folks and ask them to look at updating that asynchronously.

I've now submitted #160250 .

With that change together with #160106 , the suggestions now show the signatures for read and read_buf, and until we stabilize read_buf it'll indicate that method is unstable, so that the user doesn't get confused by it presenting both read and read_buf on equal footing.

I would propose that once #160250 goes in we unblock and merge this PR.

Separately: cc @rust-lang/rust-analyzer to request support for handling and testing suggestions from rust-analyzer. I don't think we should treat that as a blocker, though.

Separately: cc @rust-lang/rust-analyzer to request support for handling and testing suggestions from rust-analyzer. I don't think we should treat that as a blocker, though.

IIUC, we should suggest implementing either one of read and read_buf modulo stability (or both if the current toolchain is nightly) and then check whether applying autofix is correct on the selected function name and its signature, on top of this PR and #160250. Would this be correct?

Separately: cc @rust-lang/rust-analyzer to request support for handling and testing suggestions from rust-analyzer. I don't think we should treat that as a blocker, though.

IIUC, we should suggest implementing either one of read and read_buf modulo stability (or both if the current toolchain is nightly) and then check whether applying autofix is correct on the selected function name and its signature, on top of this PR and #160250. Would this be correct?

Sounds roughly right.

Status:

• rustc_must_implement_one_of now flags any unstable suggestions, so users will no longer be confused about which method they should implement.
• rustc_must_implement_one_of now provides the method signatures along with suggestions, making it easy to copy/paste and implement.
• rustc_must_implement_one_of now has a draft reference PR, satisfying the note in Allow only implementing Read::read_buf #106643 (comment) .
• I'd propose that we let rust-analyzer handle improvements to r-a suggestions asynchronously.
• We're evaluating stabilization of read_buf, which this would support.

As a result, I think we can now go ahead with this PR.

@bors r+ rollup

📌 Commit 39429fd has been approved by joshtriplett

It is now in the queue for this repository.

I have another concern about the implementation. Given that unlike read, read_buf may return both some bytes and an error, there is a choice to make about which to drop in this case.

The current implementation does the easy thing and returns the error, but I think that dropping some bytes of the reader is the wrong thing to do. On the contrary, swallowing the error is more likely to be OK, since if it was an important error it is more likely to be returned too in the next call.

@a1phyr You raised that on the tracking issue for read_buf, at #78485 (comment) . In response to that, we updated the documentation, which now states "This method makes it possible to return both data and an error but it is advised against.".

I'm curious, can this also be used for Drop::drop/pin_drop, that is currently treated specifically?

Also, @joshtriplett but also others, as a member of T-rust-analyzer it will help us if the code will mark the "preferred" method to implement in case none are (for quickfixes like "add missing impl members"). It can come in the form of a separate attribute (perhaps under the rust_analyzer:: tool namespace) or by stating that the order of the names in rustc_must_implement_one_of is the preferred order (which will also be able to account for things like the preferred method being unstable). We can special-case Read of course, but we would prefer a general approach.